http: Allow http filters to add a body that's not readily available to a headers-only request/response by yosrym93 · Pull Request #12832 · envoyproxy/envoy

yosrym93 · 2020-08-26T23:55:28Z

Commit Message:
Allow HTTP filters to add a body that's not readily available to a headers-only request/response.
Signed-off-by: Yosry Ahmed yosryahmed@google.com

Additional Description:
This is done through adding a new FilterHeadersStatus::ContinueAndDontEndStream. This allows a filter to continue headers iteration without ending the stream. The filter can then add the request/response body by using injectEncodedDataToFilterChain/injectDecodedDataToFilterChain.

Risk Level: ?
Testing: Unit and integration tests.
Docs Changes: N/A
Release Notes: N/A
Fixes #12814

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

… body Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

include/envoy/http/filter.h

yosrym93 · 2020-08-27T00:03:46Z

@mattklein123 This is a draft PR for the feature we discussed in #12814. Please take a look and let me know what you think. I added a unit test, and I'll add integration tests once we agree on the implementation.

source/common/http/filter_manager.cc

mattklein123

At a high level this looks great, thanks. Can you also add an integration test per discussion?

/wait

include/envoy/http/filter.h

source/common/http/filter_manager.cc

…t it Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

yosrym93 · 2020-08-28T01:15:11Z

I updated the doc of the new FilterHeadersStatus to explain that using add... is not supported yet, with a TODO on how to support it. I also added a message to the ASSERT that fails to make it clear that it fails because continue is called while the IterationState is already Continue. Hopefully this makes the failure clear in this case.
Working on the integration test now.

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

…ream Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

test/common/http/conn_manager_impl_test.cc

mattklein123

Thanks great work. Just some small comments. @snowp @alyssawilk in case you want to take a quick look.

/wait

source/common/http/filter_manager.cc

test/common/http/conn_manager_impl_test.cc

test/integration/http2_integration_test.cc

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

…r a filter that returns ContinueAndEndStream Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

yosrym93 · 2020-09-01T03:34:01Z

I moved the test to protocol_integration_tests and removed the default switch statement.

I also fixed the bug, done some minimum refactoring in FilterManager::decodeHeaders to be consistent with FilterManager::encodeHeaders, and updated the FilterContinueAndEndStream* tests in conn_manager_impl_test.

yosrym93 · 2020-09-01T05:50:11Z

/retest

repokitteh-read-only · 2020-09-01T05:50:17Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #12832 (comment) was created by @yosrym93.

see: more, trace.

mattklein123

Thanks looks great. @snowp can you PTAL since you have been doing a bunch of work in this area recently? Thank you.

snowp

Thanks this seems like a great addition, just a few comments

include/envoy/http/filter.h

source/common/http/filter_manager.cc

test/integration/filters/continue_headers_only_inject_body_filter.cc

test/common/http/conn_manager_impl_test.cc

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

snowp

Just a few more comments, thanks for iterating!

include/envoy/http/filter.h

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

snowp

LGTM, thanks!

mattklein123 · 2020-09-04T04:34:41Z

Awesome. Can you merge main? It was broken temporarily.

/wait

…dont-end-stream

yosrym93 · 2020-09-04T06:13:40Z

Awesome. Can you merge main? It was broken temporarily.

/wait

Done!

yosrym93 added 2 commits August 26, 2020 23:51

Added FilterHeadersStatus::ContinueAndDontEndStream

509d082

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

Added a unit test for a filter not ending the stream then injecting a…

ed657eb

… body Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

yosrym93 commented Aug 26, 2020

View reviewed changes

include/envoy/http/filter.h Outdated Show resolved Hide resolved

yosrym93 commented Aug 27, 2020

View reviewed changes

source/common/http/filter_manager.cc Outdated Show resolved Hide resolved

mattklein123 self-assigned this Aug 27, 2020

mattklein123 requested changes Aug 27, 2020

View reviewed changes

include/envoy/http/filter.h Outdated Show resolved Hide resolved

source/common/http/filter_manager.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Aug 27, 2020

yosrym93 added 3 commits August 28, 2020 01:10

Added documentation to explain a not supported case and how to suppor…

cda48f2

…t it Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

Replaced cascaded if else with a switch

d5b751c

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

Spelling fix

558d42f

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

repokitteh-read-only bot removed the waiting label Aug 28, 2020

format fix

25d069d

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

mattklein123 added the waiting label Aug 28, 2020

yosrym93 added 2 commits August 29, 2020 01:40

Added a test filter and an integration test

6399b3b

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

Added ASSERTs to make sure filters do not misuse ContinueAndDontEndSt…

20b5560

…ream Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

repokitteh-read-only bot removed the waiting label Aug 29, 2020

yosrym93 marked this pull request as ready for review August 29, 2020 01:44

yosrym93 requested a review from mattklein123 August 29, 2020 01:45

Fixed a bug in a test that the ASSERTs detected

55118fd

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

yosrym93 commented Aug 29, 2020

View reviewed changes

test/common/http/conn_manager_impl_test.cc Show resolved Hide resolved

yosrym93 commented Aug 29, 2020

View reviewed changes

test/common/http/conn_manager_impl_test.cc Show resolved Hide resolved

mattklein123 requested changes Aug 31, 2020

View reviewed changes

source/common/http/filter_manager.cc Outdated Show resolved Hide resolved

test/common/http/conn_manager_impl_test.cc Show resolved Hide resolved

test/common/http/conn_manager_impl_test.cc Show resolved Hide resolved

test/integration/http2_integration_test.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Aug 31, 2020

yosrym93 added 2 commits September 1, 2020 02:30

Moved the integration test to protocol_integration_test

77f3683

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

Fixed a bug where encodeComplete or decodeComplete were not called fo…

3bd64c1

…r a filter that returns ContinueAndEndStream Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

repokitteh-read-only bot removed the waiting label Sep 1, 2020

yosrym93 requested a review from mattklein123 September 1, 2020 03:34

mattklein123 previously approved these changes Sep 1, 2020

View reviewed changes

mattklein123 assigned snowp Sep 1, 2020

snowp suggested changes Sep 3, 2020

View reviewed changes

nits

8e518bd

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

yosrym93 dismissed mattklein123’s stale review via 8e518bd September 3, 2020 05:27

FilterHeadersStatus doc updates

5af837c

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

yosrym93 requested a review from snowp September 3, 2020 18:44

yosrym93 mentioned this pull request Sep 3, 2020

Documented a common integration tests problem #12933

Merged

snowp suggested changes Sep 3, 2020

View reviewed changes

include/envoy/http/filter.h Outdated Show resolved Hide resolved

include/envoy/http/filter.h Outdated Show resolved Hide resolved

Docs update

0dd4109

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

yosrym93 requested a review from snowp September 3, 2020 22:29

yosrym93 added 2 commits September 3, 2020 22:32

Docs update

89cbdbf

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

Revert last docs update and elaborate trailers support state

e96f658

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

snowp approved these changes Sep 3, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 4, 2020

Merge remote-tracking branch 'upstream/master' into headers-continue-…

8a74edc

…dont-end-stream

repokitteh-read-only bot removed the waiting label Sep 4, 2020

mattklein123 approved these changes Sep 4, 2020

View reviewed changes

mattklein123 merged commit 6aedd25 into envoyproxy:master Sep 4, 2020

yosrym93 deleted the headers-continue-dont-end-stream branch September 4, 2020 22:03

yosrym93 mentioned this pull request Sep 11, 2020

caching: Stream cached responses in chunks and handle downstream backpressure #13054

Closed

Conversation

yosrym93 commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

yosrym93 commented Aug 27, 2020

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yosrym93 commented Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yosrym93 commented Sep 1, 2020

Uh oh!

yosrym93 commented Sep 1, 2020

Uh oh!

repokitteh-read-only bot commented Sep 1, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Sep 4, 2020

Uh oh!

yosrym93 commented Sep 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yosrym93 commented Aug 26, 2020 •

edited

Loading

yosrym93 commented Aug 28, 2020 •

edited

Loading